Update ProductsRemote.loadAllProducts to call async/await enqueue method for response parsing to be in a background thread
#15743
Conversation
…ethod for response parsing to be in a background thread.
jaclync
added
type: task
|
|
iamgabrielma
left a comment
iamgabrielma
left a comment
There was a problem hiding this comment.
LGTM! Just a non-blocker comment/question in one of the tasks
| excludedProductIDs: excludedProductIDs, | ||
| shouldDeleteStoredProductsOnFirstPage: shouldDeleteStoredProductsOnFirstPage, | ||
| onCompletion: onCompletion) | ||
| Task { @MainActor in |
There was a problem hiding this comment.
In one of my test runs I did encounter a concurrency warning on ProductAttribute+ReadOnlyConvertible but I have not been able to reproduce it again. From my initial check, it seemed to come from here so I've tried to play around with removing the call to @MainActor in this task, but I saw no difference
[...]
/woocommerce-ios/Yosemite/Yosemite/Model/Storage/ProductAttribute+ReadOnlyConvertible.swift:24 Performing I/O on the main thread can cause hangs.
This is known to cause hangs for your users.
Since this sync calls both the mapper via loadAllProducts and later upsertStoredProductsInBackground in background threads, perhaps we are able to remove the MainActor tag if not needed 🤔
There was a problem hiding this comment.
In one of my test runs I did encounter a concurrency warning on ProductAttribute+ReadOnlyConvertible but I have not been able to reproduce it again.
Did the stack trace start from accessing ResultsController.fetchedObjects? I've been seeing ProductAttribute+ReadOnlyConvertible warnings in trunk and the PR branch for a while. I think it's from Product's relationships being loaded on the main thread (they were probably not loaded in storage before that) when converting the storage object to readonly struct. And there are several relationships of the Product model, all of them can have any number of related objects. I don't see an issue yet, will create one.
Since this sync calls both the mapper via
loadAllProductsand laterupsertStoredProductsInBackgroundin background threads, perhaps we are able to remove the MainActor tag if not needed 🤔
The @MainActor only puts the non-awaiting code on the main thread, in this case the completion callbacks. I think this is expected and how it works before, this PR just ensures the networking code, particularly the response decoding part, is not on the main thread.
There was a problem hiding this comment.
Created an issue about the performance warning: WOOMOB-619
There was a problem hiding this comment.
Did the stack trace start from accessing ResultsController.fetchedObjects? I've been seeing ProductAttribute+ReadOnlyConvertible warnings in trunk and the PR branch for a while.
I'm not sure, but now that you mention it I might have seen the same warning outside this PR as well before 🤔
Thanks for logging it in!
The @mainactor only puts the non-awaiting code on the main thread, in this case the completion callbacks. I think this is expected and how it works before, this PR just ensures the networking code, particularly the response decoding part, is not on the main thread.
👍
jaclync
deleted the
backlog/WOOMOB-607-update-ProductsRemote.loadAllProducts-to-async-await
branch
Fixes WOOMOB-607
Just one reviewer is required.
Why
In most of the network calls except for the
enqueue<M: Mapper>(_ request: Request, mapper: M) async throws -> M.Outputversion, the response is parsed by a mapper on the main thread. When the response size is large like in the support case in WOOMOB-607 with a large amount of product metadata, the UI could become unresponsive. This PR just updates theProductsRemote.loadAllProductsnetwork call from the previous Combine/completion block basedenqueuemethod to theenqueue<M: Mapper>(_ request: Request, mapper: M) async throwsmethod, so that the response parsing along with other network related tasks can be performed in the background thread. There are already several use cases ofenqueue<M: Mapper>(_ request: Request, mapper: M) async throwsfor a while that we can consider it stable.I'm prioritizing network calls for entities with potentially large amount of data like from metadata or some collection fields. Each change like this takes 200+ diffs, so I'm making changes one at a time.
Description
This pull request refactors several methods in the
ProductsRemoteclass and its related components to adopt Swift's async/await syntax, improving readability, simplifying asynchronous code, and for the response parsing to be in a background thread. It also updates corresponding unit tests and mock implementations to align with these changes.Refactoring to async/await in
ProductsRemote:Networking/Sources/Extended/Remote/ProductsRemote.swift: Replaced the completion handler inloadAllProductswith async/await and updated the method signature to return[Product]. Theenqueuemethod now directly returns the result instead of using a completion handler. It also adopts the genericListMapperso that we can change limit the response size for more use cases in WOOMOB-608. [1] [2] [3]Updates to dependent classes and methods:
Yosemite/Yosemite/Stores/ProductStore.swift: RefactoredsynchronizeProductsandupsertStoredProductsInBackgroundmethods to use async/await. Added a new async version ofupsertStoredProductsInBackgroundto handle product updates in a background thread. [1] [2] [3] [4] [5] [6]Unit test updates:
Networking/NetworkingTests/Remote/ProductsRemoteTests.swift: Updated all tests forloadAllProductsto use async/await syntax, removing the need for expectations and completion handlers. This includes tests for parsing responses, handling excluded IDs, and relaying errors. [1] [2] [3] [4]Mock implementation updates:
Yosemite/YosemiteTests/Mocks/Networking/Remote/MockProductsRemote.swift: Refactored the mock implementation ofloadAllProductsto support async/await, replacing completion handlers with direct return values or throwing errors. [1] [2]Additional test refactor:
Networking/NetworkingTests/Extensions/URLRequestConvertible+PathTests.swift: Updated a test method to use async/await for verifying analytics path generation.Steps to reproduce
Prerequisite: put a breakpoint in
ListMapperdecode calls in XcodeProductStore.synchronizeProducts--> it should be in a background threadScreenshots
RELEASE-NOTES.txtif necessary.